Cygwin: make create_root_entry tolerant of concurrent mount table initialization#333
Open
ilude wants to merge 1 commit intomsys2:msys2-3.6.7from
Open
Cygwin: make create_root_entry tolerant of concurrent mount table initialization#333ilude wants to merge 1 commit intomsys2:msys2-3.6.7from
ilude wants to merge 1 commit intomsys2:msys2-3.6.7from
Conversation
ilude
force-pushed
the
fix/add-item-race-condition
branch
from
2d5b155 to
170595b
Compare
When multiple Cygwin/MSYS2 processes start concurrently, create_root_entry() can fail with EPERM because two processes both attempt to populate the mount table in shared memory. user_info::create() calls open_shared(), which internally uses CreateFileMappingW and tracks whether the mapping was newly created or already existed. However, create() discards this information by calling the overload that drops the 'created' output parameter, relying instead on a spinlock in initialize() to coordinate. The spinlock has a 15-second timeout; if the initializing process is slow (e.g., internal_getpwsid triggering domain controller lookups), waiting processes time out and re-initialize, colliding on the immutable "/" mount entry. Use the open_shared() overload that returns the 'created' flag to determine whether this process should initialize the shared memory. Only the process that actually created the file mapping calls initialize(); all others find the mapping already exists and skip initialization. As a safety net, add MOUNT_OVERRIDE to create_root_entry() so that add_item() overwrites an existing "/" entry rather than failing with EPERM. This makes the initialization idempotent even if the primary fix is somehow bypassed. Signed-off-by: Mike Glenn <mglenn@ilude.com>
ilude
force-pushed
the
fix/add-item-race-condition
branch
from
170595b to
992d77e
Compare
ilude
changed the title
Author
|
Happy to submit this upstream to cygwin-patches if that's the preferred path. Opened here first since this is where I built and tested against, but understand if this belongs upstream given both files are core Cygwin code. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When multiple MSYS2 processes start concurrently,
create_root_entry()can crash with:
The
add_item()call returnsEPERMbecause it finds an existingimmutable
/mount entry andMOUNT_OVERRIDEis not set.This has been reported in several contexts where parallel MSYS2 process
startup is common:
bash.exeintermittently hanging with 2.36.1 when fetching or building source git-for-windows/git#3870Changes
mount.cc: Add
MOUNT_OVERRIDEtocreate_root_entry()This is the direct fix. If
/already exists in the mount table whencreate_root_entry()runs,add_item()now overwrites it instead ofreturning
EPERM. This makes the root mount initialization idempotent.shared.cc: Use
createdflag fromopen_shared()inuser_info::create()open_shared()already tracks whether the shared memory mapping wasnewly created or already existed (via
CreateFileMappingW/ERROR_ALREADY_EXISTS). Currentlyuser_info::create()calls theoverload that discards this flag.
This change switches to the overload that returns it, and calls
initialize()whencreatedis true. This provides an earlierinitialization path for the process that created the shared memory,
before
dll_crt0_1()reachesinitialize()via the spinlock.Note: this does not replace the existing spinlock coordination in
initialize(). The normal cold-start path throughdll_crt0_1()isunchanged. This is a supplementary improvement, not the primary fix.
Testing
Built patched
msys-2.0.dllfrom Cygwin 3.6.7 with MSYS2 patches.Tested on Windows 11 Enterprise with parallel bash spawning from
Claude Code (which fires multiple hook processes per tool invocation).
The crash was occurring consistently before the patch and has not
recurred after installing it.